fix notification bug #993
Merged
fix notification bug #993
+5
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yvanoers
requested changes
Jun 23, 2021
dkron/notifier.go
Outdated
| t := template.Must(template.New("report").Parse(templ)) | ||
| t, e := template.New("report").Parse(templ) | ||
| if e != nil { | ||
| return bytes.NewBuffer([]byte("Failed to execute template: " + e.Error())) |
There was a problem hiding this comment.
IMHO the error should be along the lines of 'Failed to parse template', to distinguish it from the other error below.
There was a problem hiding this comment.
You're right ! I change the message.
|
I think it would a good idea to make it fall back on a known-working template if it errs out somewhere. But this is beyond the scope of this PR. |
|
I agree. In other way I suggest the error message will be sent in logs, and not in the report message, because the component which will receive the message is probably not able to manage dkron internal errors. So, as you understand, this PR is just to avoir the panic. |
vcastellm
reviewed
Jun 29, 2021
There was a problem hiding this comment.
Nice one here, thanks @thierry-f-78, the only thing I would add here is also logging an error in that case.
|
ok for the log. I also suggest not triggering the webhook is an errors occurs, or rollbacking on standard message. The goal is no returning dkron internal error to external system. |
|
Can you add the missing logging here? so we can proceed with the merge, thanks! |
|
done. |
yvanoers
previously approved these changes
Jul 6, 2021
The notification template are valided at notification time with a "Must" function. So, if the templte is wrong, the final user get a panic/backtrace like the floowing. This patch fix this behavior, it compiles the template, catch the error and retirn the error in the generated message (like processing errors) this ensure the service continue working, even if notification were wrong. Jun 22 11:18:26 batch03 dkron[56662]: panic: template: report:1: function "JSEscaper" not defined Jun 22 11:18:26 batch03 dkron[56662]: goroutine 293 [running]: Jun 22 11:18:26 batch03 dkron[56662]: text/template.Must(...) Jun 22 11:18:26 batch03 dkron[56662]: /usr/local/go/src/text/template/helper.go:25 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).buildTemplate(0xc00121ce70, 0xc000381ae0, 0x98, 0xc0001e8e70, 0x18) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:72 +0x6bc Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).callExecutionWebhook(0xc00121ce70, 0xc0001e8e70, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:137 +0x86 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).Send(0xc00121ce70, 0xc0001e8e70, 0xc0011b6450, 0xc000e8c468) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:42 +0x85 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*GRPCServer).ExecutionDone(0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0011897d0, 0x0, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/grpc.go:240 +0x1495 Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/plugin/types._Dkron_ExecutionDone_Handler(0x2415f40, 0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0014b2c60, 0x0, 0x331caf8, 0xc0011897a0, 0xc001481980, 0x57) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/plugin/types/dkron_grpc.pb.go:233 +0x214 Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).processUnaryRPC(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0xc000ecc1e0, 0x4278258, 0x0, 0x0, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1217 +0x52b Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).handleStream(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0x0) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1540 +0xd0c Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00117e810, 0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240) Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:878 +0xab Jun 22 11:18:26 batch03 dkron[56662]: created by google.golang.org/grpc.(*Server).serveStreams.func1 Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:876 +0x1fd
ghost
dismissed
|
I agree. that's done. I also fix mistake in the previous version of the PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The notification template are valided at notification time with a
"Must" function. So, if the templte is wrong, the final user get a
panic/backtrace like the floowing.
This patch fix this behavior, it compiles the template, catch the
error and retirn the error in the generated message (like processing
errors)
this ensure the service continue working, even if notification were wrong.
Jun 22 11:18:26 batch03 dkron[56662]: panic: template: report:1: function "JSEscaper" not defined
Jun 22 11:18:26 batch03 dkron[56662]: goroutine 293 [running]:
Jun 22 11:18:26 batch03 dkron[56662]: text/template.Must(...)
Jun 22 11:18:26 batch03 dkron[56662]: /usr/local/go/src/text/template/helper.go:25
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).buildTemplate(0xc00121ce70, 0xc000381ae0, 0x98, 0xc0001e8e70, 0x18)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:72 +0x6bc
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).callExecutionWebhook(0xc00121ce70, 0xc0001e8e70, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:137 +0x86
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*Notifier).Send(0xc00121ce70, 0xc0001e8e70, 0xc0011b6450, 0xc000e8c468)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/notifier.go:42 +0x85
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/dkron.(*GRPCServer).ExecutionDone(0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0011897d0, 0x0, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/dkron/grpc.go:240 +0x1495
Jun 22 11:18:26 batch03 dkron[56662]: github.com/distribworks/dkron/v3/plugin/types._Dkron_ExecutionDone_Handler(0x2415f40, 0xc000ec2660, 0x331caf8, 0xc0011897a0, 0xc0014b2c60, 0x0, 0x331caf8, 0xc0011897a0, 0xc001481980, 0x57)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/git/ext/dkron/plugin/types/dkron_grpc.pb.go:233 +0x214
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).processUnaryRPC(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0xc000ecc1e0, 0x4278258, 0x0, 0x0, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1217 +0x52b
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).handleStream(0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240, 0x0)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:1540 +0xd0c
Jun 22 11:18:26 batch03 dkron[56662]: google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc00117e810, 0xc000d721c0, 0x3333ad8, 0xc001146780, 0xc0008b6240)
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:878 +0xab
Jun 22 11:18:26 batch03 dkron[56662]: created by google.golang.org/grpc.(*Server).serveStreams.func1
Jun 22 11:18:26 batch03 dkron[56662]: /Users/thierryfournier/go/pkg/mod/google.golang.org/grpc@v1.37.0/server.go:876 +0x1fd